Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVGNode Attribute Getters and Setters #70

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

wilgaboury
Copy link

Bindings for the assorted attribute getters and setters on SkSVGNode.

@wilgaboury
Copy link
Author

Wanted to put this pull request up for some initial review and feedback. I'm also planning to give it a thorough self-review this week and will likely make some small changes.

@tonsky
Copy link
Contributor

tonsky commented Sep 17, 2024

Sorry, a bit busy, will look at it next week

@wilgaboury
Copy link
Author

No worries! I know it's a big change to submit all at once, so please don't feel like you have to rush to review it.

@tonsky
Copy link
Contributor

tonsky commented Sep 23, 2024

Looks great to me, would be happy to have it. How about having a Scene demonstrating some of these new APIs? Maybe not all, but just a few?

@wilgaboury
Copy link
Author

Sounds good, I'll add a scene later this week.

@tonsky
Copy link
Contributor

tonsky commented Sep 24, 2024

From quick overview, I would love to see:

  • Use of currentColor, and passing it from outer world (e.g. SVG that uses currentColor, and we change it on hover from the scene)
  • Modifying some attribute inside SVG dynamically (also on hover?)

@wilgaboury
Copy link
Author

Right now, I don't think it's possible to pass in currentColor from the outer world. I would have to add additional bindings for SkSVGRenderContext and the specific method SkSVGDOM.renderNode (which I would like to do but is going to take more time).

That being said I've messed around with dynamically changing the root nodes fill color (which is inherited by the rest of the SVG) and making a scene that changes it on hover should be easy.

@tonsky
Copy link
Contributor

tonsky commented Sep 25, 2024

Yeah let’s do that. The idea is to demonstrate/test what is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants